Allow custom temporary NATS subjects detection #15736
Conversation
.../opentelemetry/instrumentation/nats/v2_17/internal/NatsRequestMessagingAttributesGetter.java
Show resolved
Hide resolved
| } | ||
|
|
||
| @Override | ||
| public boolean isHelperClass(String className) { |
There was a problem hiding this comment.
do I understand correctly that this isn't really needed for the javaagent instrumentation to function since this class is only used by the library instrumentation?
There was a problem hiding this comment.
Yes but Muzzle complained because the package of the OpenTelemetryDispatcherFactory is set to io.nats.client.impl. to access package-private classes.
[otel.javaagent 2025-12-26 18:28:06:156 +0000] [Test worker]
WARN io.opentelemetry.javaagent.tooling.instrumentation.MuzzleMatcher -
Instrumentation skipped, mismatched references were found: nats
[class io.opentelemetry.javaagent.instrumentation.nats.v2_17.NatsInstrumentationModule] on sun.misc.Launcher$AppClassLoader@73d16e93
[otel.javaagent 2025-12-26 18:28:06:176 +0000] [Test worker]
WARN io.opentelemetry.javaagent.tooling.instrumentation.MuzzleMatcher -
-- io.opentelemetry.javaagent.shaded.instrumentation.nats.v2_17.NatsTelemetry:66
Missing method io.nats.client.impl.OpenTelemetryDispatcherFactory#<init>(
Lio/nats/client/impl/DispatcherFactory;
Lio/opentelemetry/javaagent/shaded/instrumentation/api/instrumenter/Instrumenter;
)V
There was a problem hiding this comment.
I'm just curios why it wasn't needed previously. Can't spot what in the current PR requires adding it, but otherwise I'm fine with adding it.
|
|
||
| /** | ||
| * This class is internal and is hence not for public use. Its APIs are unstable and can change at | ||
| * any time. Exposed for {@link io.nats.client.impl.OpenTelemetryDispatcherFactory}. |
There was a problem hiding this comment.
I don't understand Exposed for {@link io.nats.client.impl.OpenTelemetryDispatcherFactory}.. This class isn't used by OpenTelemetryDispatcherFactory as far as I can tell.
|
|
||
| public static Pattern compile(String subject) { | ||
| return Pattern.compile( | ||
| "^" + subject.replace(".", "\\.").replace(">", "*").replace("*", ".*") + "$"); |
There was a problem hiding this comment.
none of the tests fail when replace(">", "*") is removed
| * @param temporaryPatterns A list of patterns. | ||
| */ | ||
| @CanIgnoreReturnValue | ||
| public NatsTelemetryBuilder setTemporaryPatterns(Collection<Pattern> temporaryPatterns) { |
There was a problem hiding this comment.
Perhaps it would be better to let users specify prefixes for temporary subjects? Or do you believe there are use cases where the more powerful matching capability of regex is needed?
There was a problem hiding this comment.
I don't know all usages but it might not always be prefixes. I guess one could use some.*.temporary.subject.> as the doc allows usage of both * and >. We can start with prefixes if it's simpler.
| * @param temporarySubjects A list of subjects. | ||
| */ | ||
| @CanIgnoreReturnValue | ||
| public NatsTelemetryBuilder setTemporarySubjects(Collection<String> temporarySubjects) { |
There was a problem hiding this comment.
Imo in its current for too much effort is needed to understand what this method does and how it differs from setTemporaryPatterns.
There was a problem hiding this comment.
I wanted a nats-friendly API where you specify the temp subjects the same way you'd do them in NATS. But This can be removed of course
There was a problem hiding this comment.
It would be best to document stuff like that. You should not expect people reading the code really know what kind of wildcards nats uses and to me it feels very unlikely that anybody looking at this method can figure out that it accepts that kind of wildcards.
| */ | ||
| private Pattern getTemporaryPattern(NatsRequest request) { | ||
| if (request.getSubject().startsWith(request.getInboxPrefix())) { | ||
| return NatsSubjectPattern.compile(request.getInboxPrefix() + "*"); |
There was a problem hiding this comment.
yes, I suspect you could avoid compiling the regular expression
2 participants
In order to avoid this, as both
_INBOXand_R_are used for temporary request/reply subjects.It can also be useful for internal operations. On my side I use custom subjects for warming up my application,
_TEMP.warmup.UUID. They pop-up in the span names and as I use a UUID I'd like to rename these ones to(temporary)too.